Fix custom status label bug #555

cojennin · 2019-11-20T21:05:21Z

We should be using the status in the Save as section of the Gutenberg editor. This change will add the status to the Save as section when a user saves the post with a custom status. There's currently no way to hook into core to do this, so we have a timeout to correct the status once it's set by core.

To validate status being set

Create a post
Change it's status to "Assigned"
Click "Save as Assigned"
Confirm that after saving, "Save as Assigned" still appears

mjangda · 2019-11-20T23:07:59Z

Note that we need to test these changes with the Classic Editor as well.

mjangda · 2019-11-21T15:00:49Z

Aside: we should revert the changes to package-lock.json, since those aren't necessary for this PR.

davisshaver · 2019-11-24T15:43:31Z

Tested on site with block editor enabled for posts and can confirm this branch addresses issue described in #553. Also confirmed the Move to Trash error described above exists on master and is addressed by the same branch..

I also tested with post type that does not have block editor enabled but does have custom statuses enabled. Can confirm that bugs with scheduling and trashing posts does not seem to be present on master. Additionally, no regression with scheduling and trashing experienced was experienced with this classic editor post type when testing with fix/custom-status-scheduled.

davisshaver · 2019-11-24T15:44:33Z

modules/custom-status/custom-status.php

-					'name' => esc_js( $status->name ),
-					'slug' => esc_js( $status->slug ),
-					'description' => esc_js( $status->description ),
+			$all_statuses = array_merge( $custom_statuses, $this->get_default_statuses() );


+1 to abstracting this into get_default_statuses()

Still kinda confusing, but hoping to move in the right direction

davisshaver · 2019-11-24T15:45:55Z

modules/custom-status/custom-status.php

+	 * The default statuses from core. Returned as an array of stdClass to 
+	 * make it easy to merge with our custom statuses (which are WPTerm objects)
+	 * 
+	 * return array an array of stdClass objects


missing @ in return

Ah, good catch

davisshaver · 2019-11-24T15:54:02Z

blocks/src/custom-status/block.js

+// Gutenberg will also override the status set in '.editor-post-save-draft' after save, and there isn't yet a way
+// to subscribe to a "post save" message. So instead set a timeout and override the text in '.editor-post-save-draft'
+let schedulePostStatusUpdater = () => {
+  setTimeout(() => {


Potentially a more "native" way of doing this would be to monitor for changes to the modified time (set modified time on load and compare/update/fire action in wp.data.subscribe callback).

Yea, not a bad thought, I'll mess around and see if I can get that working

Though I'm not sure it'll solve the underlying issue. We can observe post update, but the problem is here.

Gutenberg is setting a timeout of 1s to flip the message from "Saving..." to the original text: "Save as Draft|Pending". But if there's concern over about polling, we could set one timeout for something like 1.2s and that might solve it (but small race condition)

I get what you mean. The two options you proposed (callback loop and timeout after save) make sense to me based on current Gutenberg code. This seems like it could also be a good place for an upstream change to Gutenberg if there's a filter that could make it work. Best idea I have right now would be providing a way to filter/bring your own function to use instead of isPending ? __( 'Save as Pending' ) : __( 'Save Draft' );

davisshaver · 2019-11-24T15:56:45Z

modules/custom-status/custom-status.php

+		$scheduled_status->name = __( 'Scheduled', 'edit-flow' );
+		$scheduled_status->slug = 'future';
+		$scheduled_status->description = '';
+


minor but might be an extra linebreak here?

davisshaver · 2019-11-24T15:57:08Z

modules/custom-status/custom-status.php

+		return [ $published_status, $private_status, $scheduled_status ];
+	}
+
+	/***


Minor but maybe an extra asterisk here?

mjangda · 2019-11-25T03:30:36Z

modules/custom-status/custom-status.php

@@ -452,7 +434,7 @@ function post_admin_header() {
 			// Now, let's print the JS vars
 			?>
 			<script type="text/javascript">
-				var custom_statuses = <?php echo json_encode( $all_statuses ); ?>;
+				var custom_statuses =  JSON.parse( decodeURIComponent( '<?php echo rawurlencode( wp_json_encode( $statuses_js ) );  ?>' ) ); 


This seems overly complex? Why do we need multiple levels of encoding and decoding?

If there's a valid reason, we should note that in an inline comment.

Same, but it's in the suggested VIP code sample for encoding JSON. Down to move it back to just wp_json_encode if no concerns there

mjangda · 2019-11-25T04:24:24Z

blocks/src/custom-status/block.js

 }

+schedulePostStatusUpdater();


I'm a bit worried out the unwanted (and unexpected) effects of having an infinite loop to fix the label bug (i.e. where the save button occasionally flips to "Save Draft" after saving).

It would be better to keep the change to have the save button as generic as possible (i.e. "Save") and live with the occasional text flip. And then work with the Gutenberg project to find a better way to fix this.

Yea, maybe there's a better way to approach this while waiting for Gutenberg, I'll mess around a bit. Just seems like a point of confusion for consumers

mjangda · 2019-11-25T04:55:55Z

@cojennin I have a much simpler fix in #556 that I'm going to push out. I'm going to keep this open as I think it's worth iterating on to improve the the "Save" button behavior as it's definitely not ideal right now (@davisshaver's idea is worth exploring, for example).

cojennin · 2019-11-25T12:50:27Z

Yea, an upstream change to Gutneberg would definitely be preferred (less work for us!). In the meantime, there might be a better way than this infinite polling approach, it’s worth hacking around on to see what we can do

…

On Mon, Nov 25, 2019 at 7:38 AM Davis Shaver ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In blocks/src/custom-status/block.js <#555 (comment)>: > // Hack :( // @see WordPress/gutenberg#3144 -let <WordPress/gutenberg#3144-let> sideEffectL10nManipulation = status => { - let node = document.querySelector('.editor-post-save-draft'); - if ( node ) { - document.querySelector( '.editor-post-save-draft' ).innerText = `${ __( 'Save' ) }` - } +// Gutenberg will also override the status set in '.editor-post-save-draft' after save, and there isn't yet a way +// to subscribe to a "post save" message. So instead set a timeout and override the text in '.editor-post-save-draft' +let schedulePostStatusUpdater = () => { + setTimeout(() => { I get what you mean. The two options you proposed (callback loop and timeout after save) make sense to me based on current Gutenberg code. This seems like it could also be a good place for an upstream change to Gutenberg if there's a filter that could make it work. Best idea I have right now would be providing a way to filter/bring your own function to use instead of isPending ? __( 'Save as Pending' ) : __( 'Save Draft' ); — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#555?email_source=notifications&email_token=AAOM7GCHGWNJBY3MJJO6UKLQVPBMXA5CNFSM4JPZJQAKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCM2RO5I#discussion_r350158817>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOM7GGV2FEQOBKF6NVKVUTQVPBMXANCNFSM4JPZJQAA> .

cojennin · 2019-11-25T14:15:43Z

@mjangda that fix looked good (#556)! It was what I had originally started with before trying to see if we could resolve some of these other issues

cojennin · 2019-11-25T15:00:46Z

@mjangda @davisshaver doesn't seem like there's a real clean way to overcome Gutenberg explicitly setting Save as Draft or Save as Pending after a user clicks save after a 1s timeout.

I was thinking we might be able to use a MutationObserver instead of polling, but browser support isn't great (see IE), and the shim I found effectively does exactly what this PR does now (set's a timeout to observer a DOM change).

I added an attempts parameter to the timeout function to prevent looping indefinitely.

It would be great to get a fix into Gutenberg core for this issue, but in the meantime it'd be great to solve this for Edit Flow users. The behaviour we have now, where the text flips from "Save" to "Save Draft" after a user clicks save, regardless of status, is confusing.

davisshaver · 2019-11-26T10:01:05Z

I dropped an issue into Gutenberg, linking here for visibility: WordPress/gutenberg#18743

cojennin · 2020-01-13T22:05:01Z

@mjangda @davisshaver I've updated this PR with some e2e testing to verify the PR works as expected, let me know how're you're feeling about it. I'm a little more confident now that it'll work as expected

cojennin · 2020-01-24T13:39:11Z

Closing in favor of #585

cojennin requested review from mjangda, dchymko and rebeccahum November 20, 2019 21:05

davisshaver reviewed Nov 24, 2019

View reviewed changes

davisshaver mentioned this pull request Nov 24, 2019

553: Add patch to re-enable scheduling in block ed #554

Closed

mjangda reviewed Nov 25, 2019

View reviewed changes

mjangda mentioned this pull request Nov 25, 2019

Prevent js errors when scheduling / trashing post #556

Merged

mjangda mentioned this pull request Nov 25, 2019

Missing item in EditFlowCustomStatuses prevents scheduling posts #553

Closed

cojennin force-pushed the fix/custom-status-scheduled branch from 1770fc4 to 09a399b Compare November 25, 2019 14:48

davisshaver mentioned this pull request Nov 26, 2019

Save button label should be filterable to better enable custom workflows with custom statuses WordPress/gutenberg#18743

Closed

mjangda added [Status] Needs Review [Status] In-Progress and removed [Status] Needs Review labels Dec 2, 2019

cojennin mentioned this pull request Jan 2, 2020

Dockerize and add e2e testing #562

Merged

cojennin force-pushed the fix/custom-status-scheduled branch from 09a399b to 410f4f0 Compare January 13, 2020 22:03

cojennin added [Status] Needs Review and removed [Status] In-Progress labels Jan 13, 2020

cojennin requested a review from mjangda January 13, 2020 22:03

Set the post status to the correct status after a post is saved

046814c

cojennin force-pushed the fix/custom-status-scheduled branch from 23e0a65 to 046814c Compare January 14, 2020 03:45

Fix up PHPCS

b5eb91d

cojennin changed the title ~~Fix custom status scheduling issue~~ Fix custom status label bug Jan 17, 2020

cojennin mentioned this pull request Jan 24, 2020

Keep save button text updated, without errors. #585

Merged

cojennin closed this Jan 24, 2020

cojennin deleted the fix/custom-status-scheduled branch January 29, 2020 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix custom status label bug #555

Fix custom status label bug #555

cojennin commented Nov 20, 2019 •

edited

Loading

mjangda commented Nov 20, 2019

mjangda commented Nov 21, 2019

davisshaver commented Nov 24, 2019

davisshaver Nov 24, 2019

cojennin Nov 25, 2019

davisshaver Nov 24, 2019

cojennin Nov 25, 2019

davisshaver Nov 24, 2019 •

edited

Loading

cojennin Nov 25, 2019

cojennin Nov 25, 2019

davisshaver Nov 25, 2019

davisshaver Nov 24, 2019

davisshaver Nov 24, 2019

mjangda Nov 25, 2019

cojennin Nov 25, 2019 •

edited

Loading

mjangda Nov 25, 2019

cojennin Nov 25, 2019

mjangda commented Nov 25, 2019

cojennin commented Nov 25, 2019 via email

cojennin commented Nov 25, 2019 •

edited

Loading

cojennin commented Nov 25, 2019

davisshaver commented Nov 26, 2019

cojennin commented Jan 13, 2020

cojennin commented Jan 24, 2020

		}

		schedulePostStatusUpdater();

Fix custom status label bug #555

Fix custom status label bug #555

Conversation

cojennin commented Nov 20, 2019 • edited Loading

To validate status being set

mjangda commented Nov 20, 2019

mjangda commented Nov 21, 2019

davisshaver commented Nov 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davisshaver Nov 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cojennin Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjangda commented Nov 25, 2019

cojennin commented Nov 25, 2019 via email

cojennin commented Nov 25, 2019 • edited Loading

cojennin commented Nov 25, 2019

davisshaver commented Nov 26, 2019

cojennin commented Jan 13, 2020

cojennin commented Jan 24, 2020

cojennin commented Nov 20, 2019 •

edited

Loading

davisshaver Nov 24, 2019 •

edited

Loading

cojennin Nov 25, 2019 •

edited

Loading

cojennin commented Nov 25, 2019 •

edited

Loading